@llvm/pr-subscribers-clang

Author: Thomas Applencourt (TApplencourt)

diff --git a/clang/bindings/python/clang/cindex.py b/clang/bindings/python/clang/cindex.py
index d352373e85c60..b728a8d8369ad 100644
--- a/clang/bindings/python/clang/cindex.py
+++ b/clang/bindings/python/clang/cindex.py
@@ -4383,8 +4383,8 @@ def register(item: LibFunc) -> None:
 
 
 class Config:
-    library_path = None
-    library_file: str | None = None
+    library_path: str | None = os.environ.get("LIBCLANG_LIBRARY_PATH")
+    library_file: str | None = os.environ.get("LIBCLANG_LIBRARY_FILE")
     compatibility_check = True
     loaded = False
 
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 9f8d781c93021..fb812a21f2f31 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -170,6 +170,9 @@ Clang Python Bindings Potentially Breaking Changes
   ElaboratedTypes. The value becomes unused, and all the existing users should
   expect the former underlying type to be reported instead.
 - Remove ``AccessSpecifier.NONE`` kind. No libclang interfaces ever returned this kind.
+- Added the environment variables ``LIBCLANG_LIBRARY_PATH`` and ``LIBCLANG_LIBRARY_FILE``,
+  which allow users to specify the directory path and the exact library file that
+  should be used to locate libclang.
 
 What's New in Clang |release|?
 ==============================

For more rational on the change see the issue #167421.

The PR is currently in the "minimal" stage. I can:

• In the test remove the old support for CLANG_LIBRARY_PATH, and add in the release note that people should migrate to LIBCLANG_LIBRARY_PATH (will remove 3 lines per tests file so simplifying the code base)
• Add tests. The simple one will be to do a negative testing (aka setting LIBCLANG_LIBRARY_PATH to /dev/null and verify that it crash). Other test seem harder (1/ verify that cindex if working 2/ find the libclang.so that he used, 3/ use this path to set LIBCLANG_LIBRARY_PATH, retry)

Curious what do people thinks (previous CLANG_LIBRARY_PATH didn't get any test, but it was used only the tests)

CC @Endilll

✅ With the latest revision this PR passed the Python code formatter.

Ok, so:

• Added new tests: clang/bindings/python/tests/cindex/test_environment_variable.py
• Removed the old environments notions (update the README and the CINDEX error message)
• Updated the error message

Sorry I failed my git amend so the commit history is ugly, will make the review harder.

Erf, I run black I guess darker is not black : ( Will fix!

I'm not a fan of the existing terminology in this part of our codebase, where "path" should be understood as "search path" or "containing directory", while "file" and "filename" are "path to the file" (probably a full path, but that is yet to be confirmed).
That's probably out of scope here, but I'd like to see Config.library_path, Config.library_file, Config.get_filename to be renamed to something less confusing.

Totally agree! I just followed the name of the Config.set_* function for the env, but I find it confusing too! And finding good name is hard, I never really find good guideline of the naming of path.

Thanks for the comment, far better indeed. Merged them (in N commit because I don't know how to use github UI...)

Thanks for the comment, far better indeed. Merged them (in N commit because I don't know how to use github UI...)

I don't think there is a way to batch applications of suggested changes, so no worries here.
You might need to reflow the text, though.

You might need to reflow the text, though.

Didn't seem to be required. We got lucky : ) I did added a the in you can specify the path (I checked with my native speaker office-mate, " the “the” article is needed since path is singular " 🤷🏽)

Thanks for reviews @Endilll and @DeinAlptraum!